Allow export map entries to remap back to input files for a program #47925
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
weswigham
force-pushed
the
self-name-redirects
branch
from
8558033
to
f6dc158
Compare
There was a problem hiding this comment.
Conclusion from design meeting #48095 was to enforce rootDir.
|
@andrewbranch @DanielRosenwasser you wanna re-review this? |
|
So basically you kept the same guessing algorithm to use as a fallback when we issue the |
|
Yep. |
| fragment = ensureTrailingDirectorySeparator(commonDir); | ||
| } | ||
| } | ||
| if (commonSourceDirGuesses.length > 1) { |
There was a problem hiding this comment.
If I’m reading this right, I find it troubling that if your project is sufficiently close to the disk root, the requirement of specifying rootDir is removed. It feels like where your fully-contained project directory is on disk should not affect program diagnostics—this seems like an easy way to get a compilation that succeeds in Docker because it was mounted at / but fails on the host machine, for example. I would lean toward a simple cause and effect: using X feature requires Y config, without any “weeeell technically” clauses. It also makes me wonder how much of this complexity can be removed. Why not use a simpler and more naive guess (config file path || current directory?) in the error case and save ~100 LOC?
There was a problem hiding this comment.
If I’m reading this right, I find it troubling that if your project is sufficiently close to the disk root, the requirement of specifying rootDir is removed.
So... Sort-of. If the common path between the requesting file and the target file is the system root, then, yes. Every guess will be the system root, so there'll only be one unique guess. (Because it's the only possible root directory)
I don't think "mount location independence" is an aspect of our module resolver that it actually has - node_modules lookups trivially falsify that already. (Since you could mount somewhere that a parent node_modules that was used for resolution folder no longer exists.)
There was a problem hiding this comment.
in the error case and save ~100 LOC?
Tbf, most of that is comments explaining why we don't actually know the final path and are making a guess. The actual guessing code is about 10 lines, changing the method wouldn't reduce it much, or even shrink the comment (just reduce how often it actually finds a file to pair with an error for better services behavior).
|
@DanielRosenwasser what/where/when do we want this merged? |
|
(There are conflicts now because some of these changes affect deleted baselines.) It seems like this is part of the node16/nodenext story, so we should get it merged by today IMO. |
|
@DanielRosenwasser updated. |
Fixes #46762
As the code may show, this is more of a mire than it appears at first blush. Because our output directory structure depends on the set of files we load (unless
compositeorrootDiris set), there's a circular dependency here - we need to resolve our set of input files to know what the output directory structure is, but we might need to know the output directory structure to know how to find our input files. Given that, I've employed a bit of a heuristic - in module resolution, we assume the common source directory is the top-most folder between the package file/config file and the directory we're resolving in that we can find that has a file matching the name we're looking for in it. Technically, this can never be wrong, since it'll cause us to load those top-level files, which'll, in turn, guarantee that the common source directory is at that top level. It might end up being surprising that adding anexportmap to yourpackage.jsonand using a self-name import can change your output directory structure, but in some sense it makes sense, since adding an import changes that structure, too - this import just happens to (partially) be in yourpackage.json.AFAIK, this circular dependence issue was technically observable prior to export maps - imports like
import("../../dist/other/file.js")don't resolve (to the input source) in TS today, we just see very little to no demand for fixing that issue, whereas once you couch it in terms of package exports and self names, such redirection gets a little more desirable.